20190117 prom rules endpoints #1999

jtlisi · 2020-01-17T20:20:48Z

What this PR does:

This PR implements the Prometheus API alerts and rules endpoints in ruler. It adds a GRPC service to the ruler to allow for an implementation that works when rules are sharded across multiple rulers.

Which issue(s) this PR fixes:
Fixes #1720

Checklist

Tests updated
Documentation added
CHANGELOG.md

pracucci

Good job @jtlisi ! It's the first time I'm seeing the ruler internals, so please be patient going through the comments I've left (including some questions).

pkg/ruler/ruler.go

pkg/ruler/api.go

pkg/ruler/ruler.go

pracucci

Thanks Jacob for addressing my feedback. I've done a second pass review and started doing some manual tests, and I've the feeling this PR is not production-grade yet. I would prefer if, after addressing the comments, you could do extensive manual tests before asking for the next review.

Could you also rebase this PR, please?

pracucci · 2020-01-24T14:29:18Z

pkg/ruler/api.go

+
+// RegisterRoutes registers the configs API HTTP routes with the provided Router.
+func (r *Ruler) RegisterRoutes(router *mux.Router) {
+	if r.store == nil {


Is this a real case? I can't see how it could happen.

pkg/ruler/api.go

pracucci · 2020-01-24T14:33:38Z

pkg/ruler/mock_store.go

@@ -4,6 +4,7 @@ import (
 	"context"


A couple of small things:

We usually name such files with the _mock suffix (instead of prefix)

In order to have this file compiled only with tests, you could add a _test.go suffix

I'm going to change it to store_mock_test.go lol

pkg/ruler/ruler_test.go

pkg/ruler/api.go

pracucci · 2020-01-24T16:16:27Z

pkg/ruler/ruler.go

+		interval := group.Interval()
+		groupDesc := &rules.RuleGroupDesc{
+			Name:      group.Name(),
+			Namespace: group.File(),


Two things:

Why is it called Namespace if it then maps the File in the API response (see api.go)?

Is it really a good thing exposing the path of the file inside Cortex? Looks we may disclose a sensitive information in a Cortex as a service scenario

I updated the downstream path to strip the internal cortex file path. The only thing that will be left is the namespace of the users rule group.

The reason Namespace was chosen instead of file is basically to avoid have file be an API object when interacting with Cortex. It's more semantics than anything else, however I think it is good to conceptually differ between the two.

I mention it here: https://docs.google.com/document/d/1gNKiC5L_zWQ4GGgtdhJJ6FnaZXyJJBvdGTUmF0wgEFg/edit?usp=sharing

pkg/ruler/api.go

jtlisi · 2020-01-24T21:56:56Z

@pracucci I'm only part way done refactoring this. I'll try to get it ready for another pass by Monday

jtlisi · 2020-01-28T22:11:01Z

@pracucci This should be ready for another look. The scope of this PR broadened a bit while getting the tests set up. The ruler and mapper are now explicitly passed a logger upon instantiation. Also there was some updates to the fixtures to make sure the ruler loaded it's rules before the tests are run.

pracucci

Thanks @jtlisi for addressing my feedback. I kindly ask you a bit more patience to take a look at the last comments. There are many small things, except a couple of critical ones (wrong API path and missing return err in initRuler()) 🙏

pracucci · 2020-01-29T17:56:22Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * [CHANGE] Moved `--store.min-chunk-age` to the Querier config as `--querier.query-store-after`, allowing the store to be skipped during query time if the metrics wouldn't be found. The YAML config option `ingestermaxquerylookback` has been renamed to `query_ingesters_within` to match its CLI flag. #1893 
  * `--store.min-chunk-age` has been removed
  * `--querier.query-store-after` has been added in it's place.
+* [FEATURE] Added flag `experimental.ruler.enable-api` to enable the ruler api which implements the Prometheus API `/api/v1/rules` and `/api/v1/alerts` endpoints. #1999


which implements the Prometheus API /api/v1/rules and /api/v1/alerts endpoints

True, but we export them under /api/prom/.... I would mention that as well, to make it more clear to the user.

I'll make it clear it is prefixed with the configured -http.prefix.

docs/apis.md

pracucci · 2020-01-29T17:59:52Z

pkg/cortex/modules.go

@@ -456,7 +461,7 @@ func (t *Cortex) initAlertmanager(cfg *Config) (err error) {

 	// TODO this clashed with the queirer and the distributor, so we cannot
 	// run them in the same process.
-	t.server.HTTP.PathPrefix("/api/prom").Handler(middleware.AuthenticateUser.Wrap(t.alertmanager))
+	t.server.HTTP.PathPrefix(cfg.HTTPPrefix).Handler(middleware.AuthenticateUser.Wrap(t.alertmanager))


Is it worth to mention it in the changelog as a separate entry? I guess it's an [ENHANCEMENT]. Not sure, just asking.

I'm not sure when HTTPPrefix was added. I think the best thing to do would be to create a small PR that ensures everything is configured using the Prefix and adding an [ENHANCEMENT] to the change log clarifying it then. I'll revert the prefix for anything unrelated to the ruler in this PR for now.

pracucci · 2020-01-29T18:02:02Z

pkg/ruler/api.go

+	"github.com/cortexproject/cortex/pkg/util"
+)
+
+// RegisterRoutes registers the configs API HTTP routes with the provided Router.


registers the configs API > registers the ruler API

pracucci · 2020-01-29T18:12:14Z

pkg/cortex/modules.go

@@ -415,11 +415,16 @@ func (t *Cortex) initRuler(cfg *Config) (err error) {
 	cfg.Ruler.Ring.ListenPort = cfg.Server.GRPCListenPort
 	queryable, engine := querier.New(cfg.Querier, t.distributor, t.store)

-	t.ruler, err = ruler.NewRuler(cfg.Ruler, engine, queryable, t.distributor)
+	t.ruler, err = ruler.NewRuler(cfg.Ruler, engine, queryable, t.distributor, prometheus.DefaultRegisterer, util.Logger)
 	if err != nil {
 		return


This should be return err

pracucci · 2020-01-29T18:41:04Z

pkg/ruler/ruler.go

+
+	var groups []*promRules.Group
+	r.userManagerMtx.Lock()
+	mngr, exists := r.userManagers[userID]


We could compact it into the following, which also reduce the scope of variables:

if mngr, exists := r.userManagers[userID]; exists { groups = mngr.RuleGroups() }

pracucci · 2020-01-29T18:50:03Z

pkg/ruler/ruler.go

+}
+
+func (r *Ruler) getLocalRules(userID string) ([]*rules.RuleGroupDesc, error) {
+	groupDescs := []*rules.RuleGroupDesc{}


If you move this line after the locked block, you can do groupDescs := make([]*rules.RuleGroupDesc{}, 0, len(groups))

pracucci · 2020-01-29T18:54:18Z

pkg/ruler/ruler.go

-				continue
-			}
-		}
+		r.syncManager(ctx, user, filteredGroups)
 	}

 	// Check for deleted users and remove them


Note to myself: this block hasn't changed, except the logger reference.

pracucci · 2020-01-29T18:56:35Z

pkg/ruler/ruler.go

+	}
+
+	if update {
+		level.Info(r.logger).Log("msg", "updating rules", "user", "user")


Info or debug level? Up to you, just wanted to raise it, cause may be a bit verbose for an Info level.

I think an update would be a relatively rare event? Since we have a metric I'm ok with lowering it to debug though.

pkg/ruler/ruler.go

pracucci

All in all, LGTM. I haven't manually tested getShardedRules(), so I do expect you will do it before merging. I left few more nits, but they're all trivial, so no need to re-check it from my side. However, please re-request me a review if something more consistent will change 🙏

pracucci · 2020-01-30T09:38:26Z

pkg/ruler/api.go

+					State:       rl.GetState(),
+					Name:        rl.GetAlert(),
+					Query:       rl.GetExpr(),
+					Duration:    rl.For.Seconds(),


As far as it's not a breaking change, LGTM.

pkg/ruler/ruler.go

pkg/ruler/rules/compat.go

pkg/ruler/api.go

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi marked this pull request as ready for review January 18, 2020 01:36

jtlisi force-pushed the 20190117_prom_rules_endpoints branch from d198452 to ad00183 Compare January 18, 2020 01:38

pracucci reviewed Jan 21, 2020

View reviewed changes

pracucci reviewed Jan 22, 2020

View reviewed changes

pkg/ruler/ruler.go Outdated Show resolved Hide resolved

jtlisi requested a review from pracucci January 22, 2020 20:32

pracucci reviewed Jan 24, 2020

View reviewed changes

jtlisi force-pushed the 20190117_prom_rules_endpoints branch from cb310ec to 8ecebe6 Compare January 28, 2020 19:37

pracucci reviewed Jan 29, 2020

View reviewed changes

pracucci approved these changes Jan 30, 2020

View reviewed changes

jtlisi added 19 commits February 2, 2020 14:40

add api to support prom rules and alerts to ruler

c630dce

Signed-off-by: Jacob Lisi <[email protected]>

clean up deps

c7fc483

Signed-off-by: Jacob Lisi <[email protected]>

update changelog

ed010fe

Signed-off-by: Jacob Lisi <[email protected]>

update documentation

c5dfab9

Signed-off-by: Jacob Lisi <[email protected]>

register ruler routes in modules.go

142e5e1

Signed-off-by: Jacob Lisi <[email protected]>

refactor to have proper struct tags and cleaner logic for rule retrieval

43a45a9

Signed-off-by: Jacob Lisi <[email protected]>

add Rules test

48fec75

Signed-off-by: Jacob Lisi <[email protected]>

update docs

6f124cf

Signed-off-by: Jacob Lisi <[email protected]>

explicitly pass registerer to the ruler

9f14236

Signed-off-by: Jacob Lisi <[email protected]>

create respondError function

3073c81

Signed-off-by: Jacob Lisi <[email protected]>

remove unused return and err vars

79222b2

Signed-off-by: Jacob Lisi <[email protected]>

reorder registry wrapping

912599c

Signed-off-by: Jacob Lisi <[email protected]>

refactor based on pr suggestions

f5732e1

Signed-off-by: Jacob Lisi <[email protected]>

improve ruler tests for loading rules

4c6ec75

Signed-off-by: Jacob Lisi <[email protected]>

add logger to mapper tests

60e8d84

Signed-off-by: Jacob Lisi <[email protected]>

add tests for ruler api calls

80b28b2

Signed-off-by: Jacob Lisi <[email protected]>

ensure all test rulers load rules before returning

79e0925

Signed-off-by: Jacob Lisi <[email protected]>

format api_test file

3eb7425

Signed-off-by: Jacob Lisi <[email protected]>

go format

43baec1

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi added 10 commits February 2, 2020 14:41

clean mapper_test file

31b624c

Signed-off-by: Jacob Lisi <[email protected]>

add api documentation

1754949

Signed-off-by: Jacob Lisi <[email protected]>

fix alert array instantiation

c1683c3

Signed-off-by: Jacob Lisi <[email protected]>

refactor according to changes and comments on PR

1d34e66

Signed-off-by: Jacob Lisi <[email protected]>

make all time related fields nonnullable for rules protos

b00ec4b

Signed-off-by: Jacob Lisi <[email protected]>

ensure ruler is registered as grpc service

abe4ab7

Signed-off-by: Jacob Lisi <[email protected]>

use noop querier for test cases

15de020

Signed-off-by: Jacob Lisi <[email protected]>

format alert value string identical to Prometheus

53aadef

Signed-off-by: Jacob Lisi <[email protected]>

refactor per PR comments

c7fe74f

Signed-off-by: Jacob Lisi <[email protected]>

fix rebase changelog

ecf1860

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi force-pushed the 20190117_prom_rules_endpoints branch from 1009efa to ecf1860 Compare February 2, 2020 22:54

jtlisi merged commit 3d55571 into cortexproject:master Feb 4, 2020

jtlisi mentioned this pull request Mar 25, 2020

Ruler needs more comprehensive test coverage #953

Closed

jtlisi deleted the 20190117_prom_rules_endpoints branch January 26, 2021 14:16

20190117 prom rules endpoints #1999

20190117 prom rules endpoints #1999

Uh oh!

Conversation

jtlisi commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtlisi commented Jan 24, 2020

Uh oh!

jtlisi commented Jan 28, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi commented Jan 17, 2020 •

edited

Loading

jtlisi Jan 28, 2020 •

edited

Loading

jtlisi Jan 29, 2020 •

edited

Loading